-
-
Notifications
You must be signed in to change notification settings - Fork 154
GH1169 Improve parameter types for DataFrame.pct_change and Series.pct_change
#1194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
DataFrame.pct_change and Series.pct_changeDataFrame.pct_change and Series.pct_change
Dr-Irv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add tests for the various arguments in test_frame.py:test_dataframe_pct_change() ?
Dr-Irv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also add tests for Series.pct_change() in test_series.py ?
Use the check(assert_type(... pattern
|
I had to change the return type of Based on the docs, the output seems to be a series of floats, but @Dr-Irv thoughts? |
Dr-Irv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @yangdanny97
You did the right thing. |
This PR does the following for the two functions in the title:
replace kwargs with keyword-only parameters for
pct_changebased on parameters fromshiftreplace
...w/ simple defaults when applicable based on docs: https://pandas.pydata.org/docs/reference/api/pandas.Series.pct_change.htmlmake
fill_methodforSeries.pct_changebeNone, just likeDataFrame.pct_changesince all other values are deprecatedCloses type
kwargsinpct_changeaccording to params inshift#1169Tests added: Please use
assert_type()to assert the type of any return valuecc @MarcoGorelli should these have tests? they're not overloaded so I'm not sure if it's necessary